feat(edit): add, edit and remove PDF link annotations (#35) - #55
feat(edit): add, edit and remove PDF link annotations (#35)#55nonamexishere wants to merge 2 commits into
Conversation
McanKul
left a comment
McanKul
left a comment
There was a problem hiding this comment.
Thanks for the substantial work here. I found two fail-closed issues that need fixing before merge:
list_link_annotsreturns an empty list above 400 MB and silently stops at 5,000 links. The UI treats that as a complete hydration, while Save can then remove supported links that were never loaded. Please return/surface an actionable error or preserve every unhydrated annotation, with regression tests for both limits.- A no-op save deletes and recreates every supported link. That drops existing annotation properties such as
/Border,/H,/F,/AP,/QuadPoints, and/Contents, which conflicts with #35’s no-op preservation requirement. Please preserve untouched supported annotation objects (or round-trip their fields) and add a no-op structure test.
Also surface hydration failures instead of leaving Save disabled behind “Still reading links”. After #54 lands, please rebase this branch onto current development; then we can re-review the link-specific diff.
|
Thanks — the review items should be addressed on
CI should run on this head. |
7fa63ea to
9d0cb2b
Compare
|
CI has not started on this head ( The review fixes are on that commit (rebased onto On this repo, Actions looks stuck since ~15:04 UTC (jobs sitting in No rush if you are already looking at the code. |
Detect existing allowlisted URI and in-document GoTo annotations, show them on the Edit PDF canvas, and write real /Link dictionaries on save after overlay. Unsupported actions are left in place. Uses the McanKul#34 staged-output gate. Fixes McanKul#35.
list_link_annots returns AppError above 400 MiB or 5000 supported links instead of a list that looks complete. No-op Save keeps dest annot objects. Hydrate errors toast and still allow stamp-only Save.
9d0cb2b to
89ee533
Compare
McanKul
left a comment
McanKul
left a comment
There was a problem hiding this comment.
Thanks — the original review items are fixed, and both GitHub CI and my local frontend/Rust runs pass. I found three remaining blockers:
doc.objects.length === 0blocks Save after deleting the last existing link, so remove-all never reaches the backend. Please allow that case and add a regression test.- Partial hydration is not fail-closed for multi-file edits. If file A fails link listing but a link on successfully hydrated file B is edited or deleted, the frontend allows Save;
linksComplete=falsethen makesshould_rewrite_supported_linksskip all link writes, so the edit is silently discarded. Please preserve/apply per source or block link mutations while hydration is incomplete, with a mixed-source edit/delete test. - The limits conflict: listing accepts up to 5,000 links, while
validate_docrejects more than 500 total editor objects. A PDF with 501–5,000 supported links hydrates successfully but cannot save. Please align or separate link and paint-object limits and test the boundary.
Once these are covered, this should be ready for another pass.
2 participants
Summary
Implements #35: manage standard URI and in-document GoTo link annotations on Edit PDF.
This is a different job from #54 (#34 publish gate). Save still uses that gate, so this branch is stacked on it.
Depends on #54. Please merge #54 first. After that this PR rebases to link-only commits. Merging this before #54 would also land the validation gate.
In scope
/Linkannotations whose action is allowlisted URI (https/http/mailto) or in-document GoTo/Annots, then the test(editor): validate and round-trip edited PDF outputs before publish #34qpdf --check+ snapshot gateOut of scope
Why
Overlay stamps are not PDF links. Viewers need
/Subtype /Link+/A.Validation
npm run typechecknpm test(180)cargo test --manifest-path src-tauri/Cargo.toml --lib(120)Privacy Checklist